Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: Use node.js test runner #747

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test: Use node.js test runner #747

wants to merge 1 commit into from

Conversation

avaly
Copy link
Collaborator

@avaly avaly commented Feb 20, 2024 •
edited
Loading

I was interested to see how far we can get with replacing jest with the new native test runner in Node.js v20.

AFAICT The main blocker now is the fact that we can't mock ESM imports.

import * as model from '../model';
// ...

test('initialize with no models registered', (t) => {
  t.mock.method(model, 'build');

  const papr = new Papr();

  papr.initialize(db); // `model.build` inside the app code will not be mocked

  equal(model.build.mock.callCount(), 0);
});

The blocker regarding mocking modules was addressed in Node.js v22 with experimental support for module mocking.

I've rewritten all our tests using the native Node.js test runner & assert package.

@avaly avaly self-assigned this Feb 20, 2024
@avaly avaly force-pushed the test/nodejs-test-runner branch 4 times, most recently from 5fb117f to 5ab449d Compare November 11, 2024 17:38
@avaly avaly marked this pull request as ready for review November 11, 2024 17:40
@avaly avaly requested review from nunomarks and removed request for vanstinator November 11, 2024 17:43
ejmartin504
ejmartin504 reviewed Nov 14, 2024
View reviewed changes
Copy link
Collaborator

@ejmartin504 ejmartin504 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very interesting. Are we actually going to merge this? Given the "experimental" nature of it (and all the lint rules we have to disable), I don't see any reason to. What's your thinking?

src/__tests__/index.test.ts

describe('index', () => {
let db: Db;
let db: Omit<Db, 'collection' | 'collections' | 'command' | 'createCollection'> & {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

worth making an extra Mock utility type that does this for us? i.e. takes an object and re-types its properties as mocked versions?

@avaly
Copy link
Collaborator Author

avaly commented Nov 15, 2024

This is very interesting. Are we actually going to merge this? Given the "experimental" nature of it (and all the lint rules we have to disable), I don't see any reason to. What's your thinking?

I am actually planning to merge this PR soon-ish TBH.

Given that the test runner feature is marked as Stable in node.js v22, we should be safe for the future. There will only be improvements in the future for this feature. It's a pretty decent feature IMHO, for a native feature baked into the language itself.

I agree that the experimental module mocking is the only part that is standing out. I'm fairly certain that it will improve & get to a stable point in the next node.js versions. Yes, the code to achieve module mocking looks weird, but to its defense, whatever jest has for supporting ESM modules & mocking would look very similar to what we had do here.

PS: The lint rules I disabled were mostly warnings about types. I'm hopeful that types will get improved soon for the module mocks.

@avaly avaly force-pushed the test/nodejs-test-runner branch from 5ab449d to 32e3ae0 Compare November 15, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejmartin504 ejmartin504 ejmartin504 left review comments

@joshuat joshuat Awaiting requested review from joshuat

@marafat marafat Awaiting requested review from marafat

@CarlosLozanoHealthCaters CarlosLozanoHealthCaters Awaiting requested review from CarlosLozanoHealthCaters

@nunomarks nunomarks Awaiting requested review from nunomarks

At least 1 approving review is required to merge this pull request.

Assignees

@avaly avaly

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@avaly @ejmartin504